-
Notifications
You must be signed in to change notification settings - Fork 513
mercury: add variant to build perf tests #2902
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
|
@nhanford Apologies for the slow response. BUILD_TESTING_PERF was added in 2.3.0. You can add it next to that: BUILD_TESTING has always been there as far as I recall. For some reason I had the impression that this was already supported in the spack recipe... |
|
Also unrelated to that, @nhanford the error that you're getting when running that perf test with cxi is something that we've been investigating. Could you please let me know if you have seen that issue frequently when running that particular benchmark ? Thanks. |
|
@soumagne Thanks for the info. This version should be a lot cleaner. Unfortunately I was not able to test it on my system because the build failed for me due to some CMake issues, but this should work. |
|
|
||
| if "@2.3.0:" in spec: | ||
| cmake_args.append(define("BUILD_TESTING_UNIT", self.run_tests)) | ||
| cmake_args.append(define("BUILD_TESTING_PERF", self.run_tests)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could add a perf variant instead ? The idea behind having separate variables was that in most cases users want to be able to install the perf utilities but do not want to bother building the whole test suite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so having instead something like:
| cmake_args.append(define("BUILD_TESTING_PERF", self.run_tests)) | |
| cmake_args.append(define_from_variant("BUILD_TESTING_PERF", "perf")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have changed over to this approach. However, I am unable to get the perf tests to build or install this way...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah right let me try to tidy this up for you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nhanford please apply the following patch to your branch:
diff --git a/repos/spack_repo/builtin/packages/mercury/package.py b/repos/spack_repo/builtin/packages/mercury/package.py
index 1afa12b13b..29fb0db8b6 100644
--- a/repos/spack_repo/builtin/packages/mercury/package.py
+++ b/repos/spack_repo/builtin/packages/mercury/package.py
@@ -91,10 +91,12 @@ class Mercury(CMakePackage):
spec = self.spec
define = self.define
define_from_variant = self.define_from_variant
+ build_tests = self.run_tests or self.spec.satisfies("@2.3.0:+perf")
parallel_tests = "+mpi" in spec and self.run_tests
cmake_args = [
define_from_variant("BUILD_SHARED_LIBS", "shared"),
+ define("BUILD_TESTING", build_tests),
define("MERCURY_USE_BOOST_PP", True),
define_from_variant("MERCURY_USE_CHECKSUMS", "checksum"),
define("MERCURY_USE_SYSTEM_MCHECKSUM", False),
@@ -102,7 +104,6 @@ class Mercury(CMakePackage):
define_from_variant("NA_USE_BMI", "bmi"),
define_from_variant("NA_USE_MPI", "mpi"),
define_from_variant("NA_USE_SM", "sm"),
- define("BUILD_TESTING", self.run_tests),
]
if "@2.3.0:" in spec:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That works! I think we're ready to merge after checks pass!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks again for adding that!
thanks this is very useful information. If you are able to reproduce this error consistently, could you please run with the following env vars set |
Since these errors appear not to affect performance and only seem to appear on our test system, I'm going to try to run this down locally and see how far I can get, and then I will file a bug if the issue persists. Thanks! |
Thanks! that will be much appreciated as we've been trying to narrow this down... |
Hello @soumagne,
This PR adds a variant that builds and persistently installs the performance tests for Mercury.
Please let me know in which versions these CMake flags were introduced.
Preliminary results from a system similar to El Capitan at LLNL (200Gbps/HSA):
Thanks,
Nate